-
Notifications
You must be signed in to change notification settings - Fork 6.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[vcpkg_configure_cmake] Introduce REQUIRE_ALL_PACKAGES #15808
Conversation
f8da54b
to
1242929
Compare
1242929
to
8fbee6d
Compare
Very nice and very useful. Might help also reducing patchsets in some ports (and improve their maintainability too) |
Is it necessary to remove the QUIET keyword from ${ARGS}? Or putting REQUIRED at the end always wins? |
scripts/buildsystems/vcpkg.cmake
Outdated
@@ -605,20 +623,20 @@ macro(${VCPKG_OVERRIDE_FIND_PACKAGE_NAME} name) | |||
else() | |||
set(Boost_COMPILER "-vc140") | |||
endif() | |||
_find_package(${ARGV}) | |||
_find_package(${ARGV} ${_REQUIRED}) | |||
elseif("${name}" STREQUAL "ICU" AND EXISTS "${_VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}/include/unicode/utf.h") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
everything below should probably be installed within a port via a vcpkg-cmake-wrapper.cmake instead of being in the toolchain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i agree. They belong to a time in which vcpkg-cmake-wrapper.cmake was not existing...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, but I didn't want to make that change in this PR.
Just few years ago @Neumann-A (or me) would have triggered a full rebuild just after this PR was merged, enabling the REQUIRE_ALL_PACKAGES by default. And I would have really enjoyed the result of that hard work! |
We unfortunately can no longer do such "break the world" operations with the new versioning and registries features -- we've lost "total" atomicity in exchange for stability. However, we do intend to make this a requirement for all new ports and to eventually apply this policy to every current port. |
acc9cd5
to
56e849e
Compare
46de5b7
to
9dabfc5
Compare
9dabfc5
to
e9a19c1
Compare
/azp run |
f93ee2d
to
610f347
Compare
Don't want to sound needy or something, but it is a blocker for a number of PRs right now, which is a bummer. Has this stalled? |
d661858
to
f87bfb6
Compare
0f88793
to
d121552
Compare
[opencv4] Disable vendored quirc library [libsndfile] Disable Sndio dependency [armadillo] Disable PkgConfig WIP [gdal] Specify options on linux [geos][libxml2][openjpeg] .pc fixes [armadillo][gdal][geos] CR comments [pangolin] Apply REQUIRE_ALL_PACKAGES
d121552
to
aaadf6f
Compare
This PR has been inactive for a long time, please reopen it if there is any progress. |
😞 😢 😭 |
@cenit Don't worry, I believe this PR will continue. |
A recurring problem with many builds are uses of autodetected dependencies; depending on the specific install order, different CI runs results in different results for detection which results in flaky, unrobust behavior.
This PR introduces new flags to
vcpkg_configure_cmake()
to control this:REQUIRE_ALL_PACKAGES
,DISABLE_PACKAGES
, andOPTIONAL_PACKAGES
.A deficiency in the current PR implementation is that disables are also applied to transitive
find_package()
/find_dependency()
calls; this is problematic in that it (in the most general case) requires each port to calculate the transitive closure of packages used under the current configuration and compute a disable list using all that information. Ideally, we would only applyDISABLE_PACKAGES
to the first level offind_package()
, which allows these settings to only concern themselves with local information.